Repository navigation
fix: keep surrogate numeric character references from failing the mail list - #183
Conversation
…l list Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a70e5f345
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? Number.parseInt(entity.slice(2), 16) | ||
| : Number.parseInt(entity.slice(1), 10); | ||
| return code >= 0 && code <= 0x10ffff ? String.fromCodePoint(code) : original; | ||
| // Surrogates are not scalar values: fromCodePoint throws on them, and one |
There was a problem hiding this comment.
On the supported Node runtime, String.fromCodePoint(0xd800) returns an unpaired UTF-16 code unit; it does not throw (only nonintegral or out-of-range values do). Consequently, before this patch the fixture reaches Promise.all successfully and the new test fails only at its changed body assertion, so this guard/test cannot demonstrate or fix the claimed whole-inbox rejection. If that rejection is the production regression, reproduce the operation that actually throws (or reframe this as output normalization) so the real failure is not left unresolved.
Useful? React with 👍 / 👎.
jerelvelarde
left a comment
jerelvelarde
left a comment
There was a problem hiding this comment.
Reviewed 9a70e5f345051c849cf387501dbe0d863528e0e0.
Preserving surrogate numeric entities as literal text prevents introducing invalid Unicode into persisted mail. Security: the scalar guard is bounded and leaves ordinary numeric entities unchanged; no actionable security finding. I checked the earlier review: String.fromCodePoint(0xd800) does NOT throw on Node 24.17.0. It returns an unpaired surrogate. The real downstream failure is Store.put/PGlite JSONB persistence, which I reproduced as invalid input syntax for type json; this guard avoids creating that surrogate in the snippet fallback. Please correct the RangeError explanation in the description and comments; the code remains useful normalization/hardening. Verification: all 37 Google fixture tests passed, plus the independent persistence diagnostic. No live Gmail or native-device testing.
Local execution used read-only, network-disabled Linux containers with synthetic data and Node 24.17.0. Root dependency fixtures came from main’s manifest rather than the lockfile; exact-head GitHub CI covers locked full checks where green. No production credentials were supplied.
Summary
decodeSnippetdecodes numeric character references withString.fromCodePoint, which throws aRangeErrorfor surrogate code points (0xD800-0xDFFF). The range check only excluded values above0x10FFFF. A sender can put the literal text�in a message - someone discussing HTML entities, for example - and when that message falls back to its snippet, the throw rejects thePromise.allinlistMail, so one message breaks the whole inbox list. The same unpaired-surrogate input class is already handled for attachment filenames and URI encoding (wellFormed); the snippet decoder missed it.Fix
Treat surrogate code points as invalid alongside the out-of-range ones, leaving the reference literal - the same fallback the decoder already uses for values it cannot represent.
Test
a numeric character reference to a surrogate stays literal instead of failing the listfails on main with the uncaughtRangeErrorand passes with the range narrowed. The fulltests/google.test.tssuite passes (38/38), andtsc --noEmitandbiome checkare clean.